London | 25-ITP-Sep | Shaghayegh Shirinfar | Sprint 3 | Implement-and-rewrite-tests #817
Conversation
shaghayeghfar
added
the
Needs Review
Sprint-3/1-implement-and-rewrite-tests/implement/2-is-proper-fraction.js
Outdated
Show resolved
Hide resolved
Sprint-3/1-implement-and-rewrite-tests/implement/3-get-card-value.js
Outdated
Show resolved
Hide resolved
Sprint-3/1-implement-and-rewrite-tests/rewrite-tests-with-jest/2-is-proper-fraction.test.js
Outdated
Show resolved
Hide resolved
| test("should return true for a negative proper fraction (numerator < denominator)", () => { | ||
| expect(isProperFraction(-4, 7)).toEqual(true); | ||
| }); |
There was a problem hiding this comment.
What about negative improper fraction?
There was a problem hiding this comment.
I made some changes to the is-proper-fraction function , it works correctly now.
Thank you for your guides
There was a problem hiding this comment.
Your function is correct.
I meant to point out that you introduced a test for "negative proper fraction", but there is no test for "negative improper fraction". Ideally, a comprehensive test should cover all possible scenarios.
There was a problem hiding this comment.
Dear Cj
thank you very much for your reviewing and for clarification .
I changed the test to :
test("should correctly handle all negative number scenarios", () => {
const cases = [
// Proper negatives (absolute numerator < absolute denominator)
[-4, 7, true, "-4/7 proper fraction"],
[4, -7, true, "4/-7 proper fraction"],
[-2, -5, true, "-2/-5 proper fraction"],
// Improper negatives (absolute numerator >= absolute denominator)
[-5, 2, false, "-5/2 improper fraction"],
[5, -2, false, "5/-2 improper fraction"],
[-7, -4, false, "-7/-4 improper fraction"],
// Edge equal case
[-3, -3, false, "-3/-3 equal fraction"],
];
for (const [num, den, expected, desc] of cases) {
expect(isProperFraction(num, den)).toBe(expected);
}
});
cjyuan
added
Reviewed
|
Dear Cj |
|
@cjyuan I hope you’re doing well! Sorry to bother you . Many thanks, and sorry for any inconvenience! Best regards, |
There was a problem hiding this comment.
- The PR description is missing a Changelist section. Can you restore the Changelist section and include a brief description of this PR in the section?
Note: Your forgot to change the label to "Needs review" after you completed the changes. Without the label, I would not know for certain if the PR is ready to be reviewed.
| test("should return true for a negative proper fraction (numerator < denominator)", () => { | ||
| expect(isProperFraction(-4, 7)).toEqual(true); | ||
| }); |
There was a problem hiding this comment.
Your function is correct.
I meant to point out that you introduced a test for "negative proper fraction", but there is no test for "negative improper fraction". Ideally, a comprehensive test should cover all possible scenarios.
shaghayeghfar
added
Needs Review
|
Dear Cj thank you so much for your clarification and your time. I really appreciate your help and support! 🙏 |
| // Edge equal case | ||
| [-3, -3, false, "-3/-3 equal fraction"], | ||
| ]; |
There was a problem hiding this comment.
Could also consider checking -3/3 and 3/-3.
| // // Case 3: Negative fraction | ||
| // test("should return true for a negative proper fraction (numerator < denominator)", () => { | ||
| // expect(isProperFraction(-4, 7)).toEqual(true); | ||
| // }); |
There was a problem hiding this comment.
Code submitted to a PR should be free of any unused code. Keeping code clean makes code easier to review.
Should you ever need this code in the future, you can recover or access the code from previous commits. Whenever we make a commit, Git "save a copy of the whole repository" when the commit was made.
cjyuan
added
Complete
|
Dear Cj |
2 participants
Learners, PR Template
Self checklist
Changelist:
completed the sprint-3 implement-and-rewrite by answering the questions in the tasks and
writing test cases and check to make sure all test-cases have passed.